Conversation
…should not be exportable from KeyVault Co-authored-by: Keyfactor <keyfactor@keyfactor.github.io>
Added entry parameter to indicate the private key should not be exportable from KeyVault Co-authored-by: Keyfactor <keyfactor@keyfactor.github.io> --------- Co-authored-by: Joe VanWanzeele <76071503+joevanwanzeeleKF@users.noreply.github.com> Co-authored-by: Keyfactor <keyfactor@keyfactor.github.io> * cleaned up docs, split RBAC permissions into seperate file for brevity * Update generated docs * Updated changelog, nuget package references * Explicit update of Newtonsoft.Json.Bson from 1.0.2 (used by Microsoft.AspNet.WebApi.Client) to 1.0.3 to address vulnerability --------- Co-authored-by: Morgan Gangwere <470584+indrora@users.noreply.github.com> Co-authored-by: Keyfactor <keyfactor@keyfactor.github.io>
There was a problem hiding this comment.
Pull request overview
This automated merge brings the release-3.2 line into main, updating the Azure Key Vault orchestrator to support a new “NonExportable” entry parameter and reorganizing RBAC/migration documentation.
Changes:
- Added a
NonExportableentry parameter and plumbed it through the management job into Azure Key Vault certificate import policy. - Refactored and expanded documentation, including moving detailed RBAC guidance into a new
rbac.md. - Updated NuGet dependencies for the orchestrator project.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 21 comments.
Show a summary per file
| File | Description |
|---|---|
| rbac.md | New RBAC guidance and sample role definitions. |
| integration-manifest.json | Adds NonExportable entry parameter to the manifest. |
| docsource/content.md | Documentation restructure; references RBAC doc. |
| README.md | Documentation restructure; adds NonExportable docs and reworks discovery/store creation sections. |
| CHANGELOG.md | Adds 3.2.x entries and notes package/doc updates. |
| AzureKeyVault/Jobs/Management.cs | Reads NonExportable and passes it into import flow. |
| AzureKeyVault/Constants.cs | Adds EntryParameters.NON_EXPORTABLE. |
| AzureKeyVault/AzureClient.cs | Applies Exportable policy based on NonExportable during import. |
| AzureKeyVault/AzureKeyVault.csproj | Updates package versions and adds new dependency. |
| AzureKeyVault.sln | Adds rbac.md to solution items. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 1) [Configure client access; permissions and authentication](#configure-the-azure-keyvault-for-client-access) | ||
|
|
||
| 1) [Configure the Azure Keyvault for client access](#configure-the-azure-keyvault-for-client-access) | ||
|
|
||
| 1) [Create the Store Type in Keyfactor](#create-the-akv-certificate-store-type) | ||
| 1) [Create the Store Type in Keyfactor](#AKV-Certificate-Store-Type) | ||
|
|
There was a problem hiding this comment.
The TOC link fragment #AKV-Certificate-Store-Type likely doesn’t match GitHub’s generated heading id for ## AKV Certificate Store Type (which is typically lowercase, e.g. #akv-certificate-store-type). This link may be broken; update the fragment to the exact rendered heading id.
| management operations. For more information and a comparison of the two access control strategies, refer | ||
| to [this article](learn.microsoft.com/en-us/azure/key-vault/general/rbac-access-policy). |
There was a problem hiding this comment.
This markdown link is missing a URL scheme (https://). As written, learn.microsoft.com/... will be treated as a relative link and will be broken on GitHub; update it to https://learn.microsoft.com/....
| 1) Enter commma seperated list of tenant ID's in the "Directories to search" field.' | ||
|
|
There was a problem hiding this comment.
Typo/grammar: “commma seperated list of tenant ID's … field.'” contains multiple spelling issues and an extraneous trailing quote. Please correct the spelling (“comma-separated”, “tenant IDs”) and remove the stray '.
| When the Discovery job runs successfully, it will list the existing Azure Keyvaults that are acessible by our service | ||
| principal. |
There was a problem hiding this comment.
Typo: “acessible” should be “accessible”.
| | Between `11.0.0` and `11.5.1` (inclusive) | `net6.0` | | `net6.0` | | ||
| | Between `11.0.0` and `11.5.1` (inclusive) | `net8.0` | `Disable` | `net6.0` | | ||
| | Between `11.0.0` and `11.5.1` (inclusive) | `net8.0` | `LatestMajor` | `net8.0` | | ||
| | `11.6` _and_ newer | `net8.0` | | `net8.0` | | ||
| | Between `11.0.0` and `11.5.1` (inclusive) | `net8.0` | `Disable` | `net6.0` || Between `11.0.0` and `11.5.1` (inclusive) | `net8.0` | `LatestMajor` | `net8.0` | | ||
| | `11.6` _and_ newer | `net8.0` | | `net8.0` | |
There was a problem hiding this comment.
The compatibility matrix table row appears to have two rows accidentally concatenated with ||, which will break table rendering. Split this into two separate table rows so the matrix displays correctly.
| "description": "This role contains all of the necessary permissions to perform Inventory, Add and Remove operations on certificates on All KeyVaults within a Resource Group. It also contains sufficient permissions to create a new KeyVault within the resource group.", | ||
| "assignableScopes": [ | ||
| "/subscriptions/{subscriptionId1}", // allow to be applied to a specific subscription | ||
| "/subscriptions/{subscriptionId2}", // and another.. etc. | ||
| "/subscriptions/{subscriptionId}/resourcegroups/{resourceGroupName}", // allow to be scoped to a specific resource group | ||
| "/subscriptions/{subscriptionId2}/resourcegroups/{resourceGroupName2}", // and another.. | ||
| "/providers/Microsoft.Management/managementGroups/{groupId1}" // allow to be applied for all subscriptions under management group | ||
| ], |
There was a problem hiding this comment.
The custom role “JSON” examples include // comments and trailing commas (e.g., in assignableScopes), which are not valid JSON for Azure role definitions and will fail if copied directly into the portal/CLI. Consider removing the comments/trailing commas (or explicitly note that they must be removed) to make the examples copy/pasteable.
| 1) [Configure client access; permissions and authentication](#configure-the-azure-keyvault-for-client-access) | ||
|
|
||
| 1) [Configure the Azure Keyvault for client access](#configure-the-azure-keyvault-for-client-access) | ||
|
|
||
| 1) [Create the Store Type in Keyfactor](#create-the-akv-certificate-store-type) | ||
| 1) [Create the Store Type in Keyfactor](#AKV-Certificate-Store-Type) | ||
|
|
There was a problem hiding this comment.
The TOC link fragment #AKV-Certificate-Store-Type likely doesn’t match the generated heading id for ## AKV Certificate Store Type (typically lowercase, e.g. #akv-certificate-store-type). This link may be broken; update it to the exact rendered id.
| It allows the assignment of granular level, inheretable access control on both the contents of the KeyVaults, as well as higher-level | ||
| management operations. For more information and a comparison of the two access control strategies, refer | ||
| to [this article](learn.microsoft.com/en-us/azure/key-vault/general/rbac-access-policy). |
There was a problem hiding this comment.
This markdown link is missing a URL scheme (https://). As written, learn.microsoft.com/... will be treated as a relative link and will be broken on GitHub; update it to https://learn.microsoft.com/....
| "Microsoft.Authorization/roleAssignments/*", | ||
| "Microsoft.KeyVault/operations/read" | ||
| "Microsoft.KeyVault/locations/*/read", | ||
| "Microsoft.KeyVault/vaults/*/read", |
There was a problem hiding this comment.
The sample role definition has a syntax error: there is a missing comma between the Microsoft.KeyVault/operations/read and Microsoft.KeyVault/locations/*/read entries, which makes the example invalid even as JavaScript. Add the comma so the sample can be copied without errors.
| - Added an optional entry parameter to indicate whether the private key of the cert should be not exportable when stored in KeyVault | ||
| - Now specifying the pkcs12 format when wirting certs to Azure KeyVault. This should prevent the error when a PEM cert was added outside of Command and then we attempt to update without specifying the format (Azure assumes PEM and throws an error if not). |
There was a problem hiding this comment.
Spelling: “wirting” should be “writing”.
Merge release-3.2 to main - Automated PR